Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move token state into query params #1770

Open
wants to merge 86 commits into
base: master
Choose a base branch
from
Open

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Jul 19, 2024

closes FS-675

Copy link

vercel bot commented Jul 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-token-bridge ✅ Ready (Inspect) Visit Preview Dec 24, 2024 1:27pm

@brtkx brtkx changed the title feat: token query params feat: move token state into query params Jul 22, 2024

// We want to be sure this fetch is completed by the time we open the USDC modals
useCCTPIsBlocked()

const [tokenBridgeParams, setTokenBridgeParams] =
useState<TokenBridgeParams | null>(null)

useEffect(() => {
Copy link
Contributor Author

@brtkx brtkx Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused a lot of rerenders because useSelectedToken uses tokenBridge data under the hood. This block of code is not needed anyway because we set token to null when networks change

@@ -558,43 +553,9 @@ export function TokenSearch({
}

try {
// Native USDC on L2 won't have a corresponding L1 address
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this logic to useSelectedToken.

useSelectedToken now only requires erc20ParentAddress and all the computation whether it's USDC or not is handled there.

@@ -210,7 +218,7 @@ export function TransferPanel() {
)

function closeWithResetTokenImportDialog() {
setTokenQueryParam(undefined)
setTokenQueryParam(null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nicer to have null instead of undefined so that data is consistent with selectedToken

@@ -225,6 +233,36 @@ export function TransferPanel() {
connectionState
})

const isTokenAlreadyImported = useMemo(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks if token is already imported before we show the token import dialog. This way when we refresh the page we won't show the dialog all the time.

import { CommonAddress } from '../../../util/CommonAddressUtils'
import { isNetwork } from '../../../util/networks'

const commonUSDC = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to useSelectedToken as the logic is handled there


const isTestnetMode = isNetwork(networks.sourceChain.id).isTestnet

const toggleTestnetMode = useCallback(() => {
setNetworks({
sourceChainId: isTestnetMode ? ChainId.Ethereum : ChainId.Sepolia
})
}, [isTestnetMode, setNetworks])
setSelectedToken(null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a global listener for network change, so if network is changed in a different place, we don't forget to reset the selected token?

@fionnachan fionnachan linked an issue Sep 13, 2024 that may be closed by this pull request
chrstph-dvx
chrstph-dvx previously approved these changes Nov 26, 2024
Copy link
Contributor

@chrstph-dvx chrstph-dvx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants